Repository navigation
feat(gallery): resolve breakpoints from config - #31519
brandyscarney wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
09e5ae2 to
c9ad644
Compare
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! The only real issue is a gallery narrower than a configured xs above 0 ending up with no column count, which has a one-line fix. The rest are just nits.
| providedValue === undefined || sanitizedProvided === undefined ? sanitizedDefault : sanitizedProvided; | ||
|
|
||
| if (resolved !== undefined && width >= BREAKPOINTS[bp]) { | ||
| if (resolved !== undefined && width >= breakpoints[bp]) { |
There was a problem hiding this comment.
| if (resolved !== undefined && width >= breakpoints[bp]) { | |
| if (resolved !== undefined && (bp === 'xs' || width >= breakpoints[bp])) { |
Now that xs comes from config it can be above 0, and a gallery narrower than it won't match anything here. The DEFAULT_COLUMNS fallback runs through this same loop, so we end up setting --internal-gallery-columns to "undefined", which the 2 fallback in the stylesheet can't catch since the variable is set. With screenBreakpoints: { xs: 400 } and a 300px gallery, it collapses to one column with every item on its own row, and masonry skips layout.
Since the gallery measures its own width, I think one in a card or sidebar could hit this pretty easily. Letting xs always match keeps the behavior from when it was hardcoded to 0, and with it that case resolves to the xs value while the existing specs and breakpoints e2e still pass. Could you add a spec case for it too, something like xs: 400 at a width of 300?
There was a problem hiding this comment.
I can update it but this seems weird no? Shouldn't one breakpoint always be 0? Do we add an xxs that must be 0, or require xs to be 0? Should I make a follow-up for this?
Fix: 085bb4b
There was a problem hiding this comment.
I'm not sure, this is just a weird edge case that could come up I noticed. I think it's extremely unlikely, but 🤷♂️
There was a problem hiding this comment.
The default of 0 is enough, I don't think we need a follow-up. If an app moves xs above 0, what happens below it is on the app. A quick check on how other frameworks handle a raised smallest breakpoint could be good to review, though, in case there's a convention worth matching.
There was a problem hiding this comment.
I decided to remove the xs breakpoint from the screenBreakpoints config. With the way the new CSS is written, there would be no way to target the screen size under the xs breakpoint if it was larger than 0. Additionally, adding this code above just made it so that them setting it to anything above 0 was ignored for smaller breakpoints anyway: 89fbc37
c9ad644 to
21b785c
Compare
thetaPC
left a comment
There was a problem hiding this comment.
LGTM, as long as we can address FW-7285 soon
There was a problem hiding this comment.
FW-7582 isn't in FW-7285's follow-up tickets, and I think this stack is where it belongs. Could it land as a stacked PR on top of split-pane once we decide the behavior?
- Keeping the per-breakpoint default fill makes
{ md: 5 }resolve to 2, 3, 5, 6, 8, 10, so the 5 stops applying abovemd - feat(grid, col): resolve breakpoints from config #31518 resolves
col.size = { md: 6 }withresolveBreakpointMap, which carries the value upward, so col and gallery would treat the same breakpoint map shape differently once the stack merges
There was a problem hiding this comment.
I'll add it as a follow-up ticket, but I don't think it belongs in this one. This ticket changes where the breakpoints come from, making them respect the screenBreakpoints config, not how components cascade values across them. That behavior is not changed by any of my PRs.
It's also not just a decision for gallery. In ion-col, size, order and offset use resolveBreakpointMap and cascade, while the padding variables do not. The gallery resolves columns and gap in JS like size but doesn't cascade.
Worth noting the example in FW-7582 uses gap, which is spacing, and spacing doesn't cascade anywhere else in the framework either. How cascading should work for both component properties and CSS variables is worth deciding on that ticket.
| providedValue === undefined || sanitizedProvided === undefined ? sanitizedDefault : sanitizedProvided; | ||
|
|
||
| if (resolved !== undefined && width >= BREAKPOINTS[bp]) { | ||
| if (resolved !== undefined && width >= breakpoints[bp]) { |
There was a problem hiding this comment.
The default of 0 is enough, I don't think we need a follow-up. If an app moves xs above 0, what happens below it is on the app. A quick check on how other frameworks handle a raised smallest breakpoint could be good to review, though, in case there's a convention worth matching.
b026453 to
f7d11b5
Compare
f7d11b5 to
28d3d08
Compare
The gallery now uses the shared screen breakpoints and respects the
screenBreakpointsconfig. Breakpoints are still matched against the gallery's own width rather than the width of the screen, which is now noted in the properties.